Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace espree with babel. #21853

Merged
merged 7 commits into from
Apr 29, 2020
Merged

Conversation

sainthkh
Copy link
Contributor

@sainthkh sainthkh commented Apr 24, 2020

Description

Replaced espree with babel. Fixes #19952

How has this been tested?

Ran npm run docs:build and found no change.

Screenshots

N/A

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • [N/A] My code follows the accessibility standards.
  • [N/A] My code has proper inline documentation.
  • [N/A] I've included developer documentation if appropriate.
  • [N/A] I've updated all React Native files affected by any refactorings/renamings in this PR.

@oandregal
Copy link
Member

These are the testing steps I'm following;

  • In master
    • Modify the JSDoc comment of some exported entity and use the optional chaining syntax. Suggestion: modify getCurrentUser in packages/core-data/src/selectors.js.
    • Run npm install && npm run docs:build.
    • The expected result is that it should fail due to the unrecognized syntax.
    • Clean up any changes.
  • Switch to this branch:
    • Modify the JSDoc comment of some exported entity and use the optional chaining syntax. Suggestion: modify getCurrentUser in packages/core-data/src/selectors.js.
    • Run npm install && npm run docs:build
    • Expected result is that the appropiate docs will be updated. Following the suggestion above: there should be changes in docs/designers-developers/developers/data/data-core.md and packages/core-data/README.md.

As per these steps, this works as expected! Happy to see that the code changes are minimal. Going to review the code now.

Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good progress here, this is a nice well-scoped refactor!

We'd need to figure out a way for us to:

  1. use the same configuration than Gutenberg without hard-coding the path
  2. use the presets already available (that include JSX parsing)

@oandregal oandregal added the [Tool] Docgen /packages/docgen label Apr 24, 2020
@sainthkh
Copy link
Contributor Author

After your comment about presets option, it made me wonder why it doesn't exist while plugins does. A babel preset is just a package of multiple plugins. It's a bit weird to learn that you can set individual plugins while cannot set packages.

After thinking about this weirdness for a while, I realized that we don't need babel.config.js at all.

When using babel, we expect it to transpile new syntax to old syntax. To do so, babel parses js code and runs plugins to generate new one.

In other words, babel works in these 3 steps.

  1. babel/parser parses code and generates AST.
  2. plugins manipulates the AST and create new one.
  3. babel generates new code from new AST.

As the jobs of babel/parser and babel plugins are different, we don't need to import babel.config.js at all.

I removed other configs and removed them.

@sainthkh sainthkh force-pushed the update/docgen-babel branch from 3855957 to 18f6f76 Compare April 27, 2020 04:56
@oandregal
Copy link
Member

Good point about the transforms plugins. We do need the syntax/parser plugins for the parser to understand some syntax potentially used in Gutenberg: in our case, the jsx plugin as well as the numericalSeparator proposal (see full discussion).

With that change, I think we're good to go.

@oandregal
Copy link
Member

oandregal commented Apr 27, 2020

It does bother me a bit that we need to keep an additional babel config beyond the existing ones (wordpress/babel-preset-default + gutenberg babel.config.js). What if instead of using babel-parser we use @babel/core as we do in Gutenberg. 🤔

It looks like we could reuse @wordpress/babel-preset-default and won't need any extra config. If, at some point in the future, we needed also something from the Gutenberg's babel.config.js we could copy it verbatim (or even make it reusable), which sounds better than having to look into babel internals to see which proposals to enable, etc.

@sainthkh
Copy link
Contributor Author

I tested your proposal about @babel/core by adding a file at docgen/src/t.js with the code below:

const core = require( '@babel/core' );
const parser = require( '@babel/parser' );

const code = `
try {
	state?.currentUser;
} catch {
	state ?? 1_000;
}
`;

try {
	core.parse( code );
	process.stdout.write( 'core passed\n' );
} catch ( e ) {
	process.stdout.write( `core failed: ${ e }\n` );
}

try {
	parser.parse( code, {
		plugins: [ 'jsx' ],
		sourceType: 'module',
	} );
	process.stdout.write( 'parser passed\n' );
} catch ( e ) {
	process.stdout.write( `parser failed: ${ e }\n` );
}

try {
	parser.parse( code, {
		plugins: [ 'jsx', 'numericSeparator' ],
		sourceType: 'module',
	} );
	process.stdout.write( 'parser2 passed\n' );
} catch ( e ) {
	process.stdout.write( `parser2 failed: ${ e }\n` );
}

Confirmed that @babel/core works correctly without any setting. Let's continue in that way.

@sainthkh sainthkh force-pushed the update/docgen-babel branch from 18f6f76 to ac8af64 Compare April 28, 2020 06:07
@oandregal
Copy link
Member

This is nice! We also get the Gutenberg root configs for free 👏

I think we're ready to merge, the only thing left would be to document how this works in the README. Perhaps we can add a section called "Babel configuration" after the "CLI Options" that says that it takes the project-wide config, what happens if there is none, etc. What do you think?

Also, could you test that documents are generated properly with the pre-commit hook? For some reason, the pre-commit hook is not working for me and I want to make sure the pre-commit hook doesn't mess up the current working directory babel uses to find the config.

Testing instructions would be:

  • Paste the testing code within the getCurrentUser selector at packages/core-data/src/selectors.js.
  • Modify some part of its JSON comment.
  • git add packages/core-data/src/selectors.js and commit.
  • The expected result is that the commit should fail because there are two new files to add (docs/designers-developers/developers/data/data-core.md and packages/core-data/README.md).

@sainthkh sainthkh force-pushed the update/docgen-babel branch from ac8af64 to 4dc276c Compare April 29, 2020 02:50
@sainthkh
Copy link
Contributor Author

  • I confirmed that pre-commit hook generates docs properly and commit fails.
  • Updated documentation

@oandregal oandregal merged commit 6c26cdf into WordPress:master Apr 29, 2020
@oandregal
Copy link
Member

Good work here, @sainthkh!

@github-actions github-actions bot added this to the Gutenberg 8.1 milestone Apr 29, 2020
@youknowriad
Copy link
Contributor

Impressive work here :)

@nb
Copy link
Member

nb commented May 4, 2020

Just wanted to say “Thank you”, @sainthkh – I was working on some package changes including and every commit was failing, because docgen did not supporting newer ECMASript features. Not the best experience you can imagine…

Today, I rebased with master and it was all fixed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Tool] Docgen /packages/docgen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docgen: Support ES2019, approved ES2020 syntax
4 participants